[issue-3064][slice-5/6] Lifecycle-aware monitoring: health-monitor... - #3198

Review: slice-5 lifecycle-aware monitoring (#3064)

I traced every changed file and the cross-module data flow into the production health-monitor poll path and the orchestrator event loop. The convergence-stall re-homing is wired correctly, but two blocking issues make the health-monitor half of this slice non-functional in production, and the one piece that does work has no real test. Requesting changes.

(Note for the record: get_idle_budget_minutes uses except TypeError, ValueError: — I verified this is not a syntax error under the project's requires-python = ">=3.14"; PEP 758 makes the parenthesis-less form valid and it catches both. So that line is fine.)

🔴 BLOCKING 1 — Active-Job scoping is a cross-module silent no-op; orchestrator-mode heartbeat/progress tripwires are unconditionally suppressed

The entire "fire only while a Job is active" behavior hinges on HealthMonitor._active_jobs, which is populated only by set_active_roles() (health_monitor.py:227). That method has no production caller anywhere in the repo — only the new tests call it:

$ grep -rn set_active_roles --include=*.py . | grep -v /tests/ | grep -v .egg-state
orchestrator/health_monitor.py:227:    def set_active_roles(...)   # definition only

_enable_orchestrator_mode_surfaces() (concurrent_executor.py:530) calls set_orchestrator_mode(True) but never set_active_roles(...), and the live 30s poller (routes/pipelines.py:~22672, _health_monitor_poll → check_tripwires) calls check_heartbeats()/check_progress() without ever refreshing the active set. So in orchestrator mode _active_jobs is permanently empty.

Consequence — _orchestrator_skip_tripwire (health_monitor.py:464):

if not self._orchestrator_mode:
    return False
if not self._active_jobs:      # always True in production
    return True                # → skip EVERY agent
return agent_id not in self._active_jobs

With _active_jobs empty, this returns True for every agent. Both check_heartbeats (:797) and check_progress (:~910) hit this gate and continue, so all heartbeat and progress tripwires are suppressed for all roles in orchestrator mode. The snapshot-augmentation loop for role in self._active_jobs (:775) is also a permanent no-op.

This directly violates two of the slice's stated acceptance criteria:

• "tripwires fire only while that role's Job is active" → they never fire.
• "silent mid-event pod still trips" → it never trips. The augmentation that was supposed to make a never-heartbeating live pod visible (:773–778) iterates an empty set.

This is not just a missing nicety — it's a safety-coverage regression for the mode this whole effort introduces. A pod that is alive but whose agent has hung (no exit, no heartbeats) is caught by neither mechanism: the heartbeat-timeout path is suppressed here, and the convergence-stall path explicitly excludes in-flight jobs (event_loop.py:869, if key in self._live_keys: continue). So a hung mid-event pod goes undetected.

Fix: wire set_active_roles(...) from the live Job set on each poll tick (the event loop already tracks _live_keys/_key_meta, and the executor knows the spawned roles). Until something populates _active_jobs, the active-Job-scoping and silent-mid-event-pod behaviors do not exist in production regardless of how the unit tests look. If this wiring is genuinely intended for slice-6, then this slice's acceptance criteria and tests are misleading and should be reworded to claim only "no-pod ⇒ no alert," not active-Job scoping.

Related dead code from the same root cause: the container-exit ghost-suppression guard at health_monitor.py:636 (self._orchestrator_mode and self._active_jobs and agent_id not in self._active_jobs) can never be true (empty _active_jobs), so the comment above it describing ghost-container suppression documents behavior that never happens. (Container exits always escalate, which matches the test — but the comment is misleading and should be fixed alongside the wiring.)

🔴 BLOCKING 2 — The only production-wired new behavior (convergence-stall) has no behavioral test

_check_convergence_stall (event_loop.py:800) is the one genuinely-wired new path (convergence_stall_notifier=self._emit_supervision_alert, signatures match — verified). Yet the test that claims to cover it, TestOwnershipModeIdleBudgetAnomaly.test_anomaly_name_matches_in_pod_alert (test_health_monitor.py), asserts only:

assert isinstance(EVENT_PUMP_IDLE_BUDGET_MIN_DEFAULT, int)
assert EVENT_PUMP_IDLE_BUDGET_MIN_DEFAULT == 30
assert anomaly_name == "stuck-phase-transition"   # literal == itself

It never instantiates OrchestratorEventLoop, never supplies tracker-timestamp fixtures, never spies on the notifier, and never calls _check_convergence_stall. It would pass unchanged if the entire method were deleted. This is a tautological assertion that bypasses the production code path — exactly the anti-pattern the review rules call out, and it fails the slice's own acceptance criterion: "the convergence-stall re-homed idle-budget alert ... asserted from tracker-timestamp fixtures."

Fix: add real test_event_loop.py coverage that drives _check_convergence_stall with a fake tracker whose get_latest_progress_timestamp() and _derive_next_action are controlled, asserting: (a) no alert before budget, (b) stuck-phase-transition emitted once after budget elapses, (c) sticky latch (no duplicate emission), (d) reset when the bus moves, (e) suppression while key in _live_keys, (f) dormant when notifier is None.

🟡 Non-blocking

1. refresh_at_spawn is dead and doesn't do what its name/docstring imply (heartbeat.py:202). No production caller, and in orchestrator mode should_fan_out_gateway_session returns False unconditionally (:174), so the _last_fan_out cooldown it clears is never consulted. It also never contacts the gateway, so the PR's "session refresh happens at spawn time" is not backed by this method. Either wire it and have it (or the spawn path) actually refresh the session, or drop it. Confirm one-shot pods can't outlive the gateway's 60-min idle window mid-event with fan-out fully suppressed.

2. Dead reset branch at event_loop.py:876: if self._stall_alerted.get(role) and now - bus_timestamp < budget_sec: is unreachable — the all-roles reset at the top of the method (:847-ish) already clears _stall_alerted whenever now - bus_timestamp < budget_sec. Remove it or the comment.

3. Stall timer is observation-based, not event-based. _stall_first_seen[role] = now is set on first observation (:~893), so the alert fires budget_min after the loop first sees the pending event, not after it actually became pending. On a fresh/restarted loop this can delay the alert by up to a full budget window versus the in-pod check_idle_budget. Consider seeding from the event's pending timestamp.

4. Dead TypeError arm in get_idle_budget_minutes (event_loop.py:159): raw is always a str (env get + strip), so int(raw) can only raise ValueError. The sibling get_event_loop_poll_interval documents exactly this ("no TypeError arm needed"); this function should match for consistency.

— Authored by egg

Contract verification — PR #3198 (issue-3064 slice-5)

Verified the diff (origin/egg/issue-3064/slice-4..origin/egg/issue-3064/slice-5) against the slice-5 contract tasks. The headline feature — re-homing the idle-budget alert as an orchestrator-side convergence-stall judgment — is implemented well and wired correctly. However, the health-monitor active-Job scoping (task-5-1) is non-functional end-to-end, the container-exit gate contradicts its own acceptance criterion, and one task-5-2 test is vacuous so a required criterion is not genuinely asserted. Requesting changes.

Note: egg-contract / orchestrator was unreachable for the whole session, so I could not run verify-criterion. The contract on disk (.egg-state/contracts/issue-3064.json) stores criteria as free-text bullets per task, not discrete ac-N ids. Findings below map to those bullets.

✅ What checks out

• Convergence-stall re-homing (event_loop.py) — _check_convergence_stall reads EGG_BRC_IDLE_BUDGET_MIN via get_idle_budget_minutes() (default 30), emits anomaly stuck-phase-transition, which exactly matches the in-pod wrapper (orchestrator/consensus_wrapper.py:695, EVENT_PUMP_IDLE_BUDGET_MIN_DEFAULT = 30). It's wired in concurrent_executor.py:509 via convergence_stall_notifier=self._emit_supervision_alert, and the notifier's signature (*, anomaly, priority, summary, detail) matches the call site. The per-role first-seen latch + bus-activity reset are sound. This satisfies task-5-1's "pending-event-over-budget raises the existing anomaly name from the orchestrator."
• Heartbeat mode guard — should_fan_out_gateway_session returns False in orchestrator mode; set_orchestrator_mode(True) is wired in concurrent_executor._enable_orchestrator_mode_surfaces. Pod-mode default is False, so pod behavior is unchanged.
• Existing tests unmodified — both test diffs are purely additive (appended at EOF); no existing test bodies were edited. The 11 new health-monitor tests and 4 new heartbeat tests pass locally (python3 -m pytest, 3.14.6).

🔴 Blocking

1. set_active_roles() has no production caller → active-Job scoping is dead code in production (task-5-1, criterion 1).

grep over orchestrator/**.py shows set_active_roles is called only from test_health_monitor.py. Nothing in concurrent_executor.py or event_loop.py populates _active_jobs from live Job labels, despite the docstring at health_monitor.py:469 claiming "the concurrent executor or event loop sets this from live Job labels on each poll." Meanwhile set_orchestrator_mode(True) is wired, so in production:

• _active_jobs is permanently empty → _orchestrator_skip_tripwire returns True for every role → all heartbeat/progress tripwires are skipped unconditionally.
• The criterion "tripwires fire only while that role's Job is active" and "silent mid-event pod still trips" are therefore unreachable in production — the silent-pod-trips branch only executes when a role is in _active_jobs, which never happens.

The unit tests pass only because they call monitor.set_active_roles({...}) by hand. No slice wires this (slice-6 is docs-only), so this isn't deferred scope — it's a missing integration. Either wire set_active_roles from the event-loop poll site (from _live_keys / live Job labels), or, if suppressing all in-pod tripwires in orchestrator mode is the intended slice-5 behavior with convergence-stall as the sole replacement, update the task/criterion and the comments to say so (and drop the unreachable scoping logic).

2. Container-exit gate contradicts the criterion and is inconsistent with the heartbeat path (health_monitor.py:634).

if self._orchestrator_mode and self._active_jobs and agent_id not in self._active_jobs:
    return

The criterion says container-exit tripwires "apply ONLY while a Job is active for that role." But this only suppresses when _active_jobs is non-empty. With the empty _active_jobs that production always has (finding #1), the guard is false and every container exit escalates — including the ghost-container case the comment at health_monitor.py:632 says it suppresses. This is the inverse of the heartbeat path, where empty _active_jobs skips everything. The test test_orchestrator_mode_active_job_container_exit_still_trips codifies this (it never sets an active role yet asserts the alert fires), so the test contradicts the task rather than verifying it.

3. Idle-budget anomaly-name equality test is vacuous; _check_convergence_stall has zero coverage (task-5-2, criterion 2).

test_anomaly_name_matches_in_pod_alert does:

anomaly_name = "stuck-phase-transition"
assert EVENT_PUMP_IDLE_BUDGET_MIN_DEFAULT == 30
assert anomaly_name == "stuck-phase-transition"   # literal compared to itself

It never references the string event_loop.py actually emits, nor any wrapper anomaly-name constant — so it would still pass if the implementation emitted a different anomaly. The criterion "idle-budget anomaly-name equality asserted against the in-pod alert's name" is not genuinely satisfied. The task description also requires the convergence-stall alert be "asserted from tracker-timestamp fixtures," but there is no test (in test_event_loop.py or elsewhere) that drives _check_convergence_stall — the most substantive new logic (~130 lines, budget threshold, latch, notifier invocation) ships untested. (The implementation itself is correct on inspection — see "What checks out" — but the criterion asks for a test that asserts it.)

Suggested: import the wrapper constant and assert the loop emits the same string by capturing the convergence_stall_notifier call with a fake tracker whose get_latest_progress_timestamp() is older than the budget.

🟡 Non-blocking

4. refresh_at_spawn is unwired and inert under the mode guard (heartbeat.py:204). No production caller (tests only), and its stated purpose ("so the first post-spawn heartbeat is not throttled") cannot take effect in orchestrator mode, where should_fan_out_gateway_session returns False before the cooldown is ever consulted. As written it only affects pod mode, which contradicts its docstring ("called when a one-shot pod is spawned"). Clarify intent or wire it at the spawn site.

5. should_fan_out_gateway_session re-acquires self._lock (heartbeat.py:176 then :181). The orchestrator-mode early-return takes the lock, releases it, then the cooldown path takes it again. Harmless but a single with self._lock: covering both is cleaner and avoids a redundant acquire.

6. except TypeError, ValueError: (event_loop.py:159) — non-idiomatic. It's valid on the project's Python (requires-python >=3.14) and I confirmed it catches both exceptions, so it's not a bug. But the parenthesized except (TypeError, ValueError): is the conventional form and avoids surprise for readers who'll parse it as the old Python-2 as syntax. (int() on a string only raises ValueError, so TypeError is dead anyway.)

Verdict

Request changes. Findings #1–#3 mean two task-5-1 criteria (active-Job scoping / silent-pod-trips, container-exit scoping) don't hold end-to-end in production and one task-5-2 criterion isn't genuinely asserted. The convergence-stall re-homing and heartbeat mode guard are good. Once set_active_roles is wired (or the scoping behavior is intentionally narrowed and documented), the container-exit gate is made consistent, and the anomaly-name/stall tests assert against the implementation, this is close.

— Authored by egg

Contract Re-verification — slice-5 (incremental)

Re-reviewed the delta since my last verification (ddbed282), i.e. the single PR-authored commit 6b5cf0dc Wire active-Job scoping and add convergence-stall tests. Scope: git log ddbed282..HEAD --not origin/egg/issue-3064/slice-4 -p (8 files: 5 production, 3 test).

Verdict: contract-compliant. The substantive task-5-1 / task-5-2 acceptance criteria are met, no previously-verified behavior regresses, and this commit fixes a real latent defect. One AC sub-clause (refresh-at-spawn) was intentionally dropped — flagged below for your awareness, non-blocking.

What I verified

task-5-1 — lifecycle-aware monitoring

• Idle role with no pod never alerts; tripwires scope to active Jobs; silent mid-event pod still trips — met. All three tripwire paths now gate on the same _orchestrator_skip_tripwire() helper: heartbeat (health_monitor.py:801), progress (:915), and — newly in this delta — container-exit (:641, was an inline ad-hoc check before). An empty _active_jobs in orchestrator mode suppresses every role (:482); a silent mid-event pod still trips via the synthetic (role, 0.0, False) snapshot entry (:780) combined with the role being present in _active_jobs.
• Root-cause fix (the substance of this commit): event_loop.poll_once() now publishes its live-Job role set via _publish_active_roles() → active_roles_notifier → concurrent_executor._publish_active_roles() → HealthMonitor.set_active_roles() (event_loop.py:757, concurrent_executor.py:510,531). Before this wiring _active_jobs stayed permanently empty in orchestrator mode, so _orchestrator_skip_tripwire suppressed every role — the entire active-Job scoping feature was inert. This makes it functional. Role is derived correctly from _key_meta[key][1] ({key: (action, role)}, confirmed at :602/:840).
• Pending-event-over-budget raises the existing anomaly using EGG_BRC_IDLE_BUDGET_MIN — met. _check_convergence_stall reads get_idle_budget_minutes() and raises _idle_budget_anomaly_name(), single-sourced from consensus_wrapper.EVENT_PUMP_IDLE_BUDGET_ANOMALY = "stuck-phase-transition" (the wrapper template now interpolates the same constant, so in-pod and orchestrator-side alerts are guaranteed identical). The NB3 fix — seeding the stall window from bus_timestamp rather than first-observation — correctly mirrors the in-pod idle = now - LAST_PROGRESS and avoids a full-budget delay on a restarted loop. Sticky latch + all-roles bus-movement reset logic is sound (fires once per episode; bus_ts is None ⇒ now avoids first-poll false-alert).
• HeartbeatCoordinator mode guard; absent sender trips nothing; pod-mode unchanged — met. should_fan_out_gateway_session returns False in orchestrator mode under a single lock acquisition (the double-lock was collapsed — good).

task-5-2 — monitoring tests

• Both ownership modes asserted side-by-side per tripwire (container-exit split into active-trips / ghost-suppressed / pod-always-trips).
• The previously tautological anomaly-name test (assert "stuck-phase-transition" == "stuck-phase-transition") was replaced with a real cross-module default equality (_IDLE_BUDGET_MIN_DEFAULT == EVENT_PUMP_IDLE_BUDGET_MIN_DEFAULT) plus behavioral emission coverage in TestConvergenceStall (no-alert-before-budget, fires-once-after, sticky latch, bus-reset, in-flight-Job suppression, notifier-None dormancy) and TestActiveRolesPublishing. This is a genuine strengthening.

All 5 changed production files compile cleanly under python3 -m py_compile; CI is green (Unit Tests / Python / Lint / Security / Integration all SUCCESS). Per review conventions I trusted CI rather than re-running the suite.

Flag for human reviewer (non-blocking)

The task-5-1 and task-5-2 acceptance criteria — and the slice goal text — explicitly name refresh-at-spawn as a HeartbeatCoordinator deliverable. This commit removed HeartbeatCoordinator.refresh_at_spawn and its three tests entirely, as dead code.

The removal is defensible and I do not consider it blocking:

• The method was provably a no-op in both modes: in orchestrator mode should_fan_out_gateway_session short-circuits to False before the cooldown is ever consulted, so resetting that cooldown changes nothing; in pod mode the method was never called.
• The criterion's underlying intent — "in orchestrator mode session refresh happens at spawn (slice-4 session reuse)" — is satisfied structurally by slice-4's worktree-reattach / gateway-session reuse, not by this method.
• It was an explicit, reviewer-endorsed cleanup (the prior reviewer_code NACK asked to add it; the subsequent review judged it dead and asked to drop it).

I'm surfacing it only because it's a literal divergence from the written AC text. If the contract's AC wording should be treated as satisfied-by-intent here, no action is needed; otherwise the AC text may want a one-line amendment to reflect that refresh-at-spawn is owned by slice-4.

Note on criterion marking

The orchestrator was unreachable this run (egg-contract show / verify-criterion both return Orchestrator unreachable), so I could not mark criteria programmatically. The local contract snapshot is a stale refine-phase copy with an empty top-level acceptance_criteria list (no ac-N ids), so criteria live as task-level prose only — there were no ac-N ids to mark regardless.

— Authored by egg

Re-review: slice-5 lifecycle-aware monitoring (#3064) — all blocking issues resolved

I re-reviewed the delta since ddbed28 (one commit, 6b5cf0d), traced the active-Job scoping end-to-end through the production singleton, and re-ran the new/changed tests. All three blocking issues from my prior review are now properly fixed — not superficially patched. Approving.

✅ Blocking 1 (active-Job scoping was a cross-module no-op) — fixed and verified end-to-end

set_active_roles now has a real production caller. OrchestratorEventLoop.poll_once calls _publish_active_roles (event_loop.py:758), which derives the live-Job role set from _live_keys/_key_meta and hands it to active_roles_notifier → ConcurrentPhaseExecutor._publish_active_roles (concurrent_executor.py:530) → get_health_monitor().set_active_roles(roles).

I verified the two sides share state: get_health_monitor() returns the module-level singleton (health_monitor.py:1313) that init_health_monitor populates, and the live 30s poller uses that same health_monitor_instance (routes/pipelines.py:22637, :22721). _enable_orchestrator_mode_surfaces sets set_orchestrator_mode(True) on the same singleton. So _active_jobs is now populated on every event-loop tick and the gate is live.

I specifically checked the restart path: OrchestratorEventLoop.reconcile() is test-only (production seeds an empty live set and re-derives through _handle_role's spawn path, per concurrent_executor.py:467), and that spawn path populates _key_meta (event_loop.py:840) even when the spawner adopts an already-live Job by dedupe label. So _key_meta is never empty for a live role in production, and _publish_active_roles reflects active roles correctly.

✅ Blocking 2 (container-exit gate contradicted its criterion) — fixed

_on_container_stopped now routes through _orchestrator_skip_tripwire(agent_id) (health_monitor.py:643), consistent with the heartbeat/progress paths: active Job → escalates (silent mid-event pod trips), no active Job → suppressed (ghost reap). The three new tests (test_orchestrator_mode_active_job_container_exit_still_trips, test_orchestrator_mode_ghost_container_exit_suppressed, test_pod_mode_container_exit_always_trips) assert all three branches against the real gate. I confirmed normal one-shot completions don't emit CONTAINER_STOPPED (only the HITL-restart path at routes/decisions.py:134 does), so the gate change introduces no normal-path false positive.

✅ Blocking 3 (vacuous convergence-stall test) — fixed

The tautological test_anomaly_name_matches_in_pod_alert is replaced by TestConvergenceStall (test_event_loop.py), which drives _check_convergence_stall with a fake tracker + spy notifier and asserts: no alert before budget, single emission after budget, anomaly name equals the cross-module constant consensus_wrapper.EVENT_PUMP_IDLE_BUDGET_ANOMALY, sticky latch, bus-movement reset, in-flight-Job suppression, notifier-None dormancy, and agent-free roles never stall. The remaining health-monitor test now asserts a genuine cross-module equality (_IDLE_BUDGET_MIN_DEFAULT == EVENT_PUMP_IDLE_BUDGET_MIN_DEFAULT). I ran the new tests locally: 11 passed (TestConvergenceStall + TestActiveRolesPublishing), 13 passed (TestOwnershipModeHeartbeatMatrix + TestOwnershipModeIdleBudgetAnomaly).

✅ Non-blocking items all addressed

• NB3 event-based seeding: _stall_first_seen[role] = bus_timestamp (not now) — verified a freshly-started loop alerts immediately when now - last_bus_activity already exceeds budget rather than waiting a full extra window.
• Dead per-role reset branch: removed (correctly — the all-roles reset at the top already covers it).
• refresh_at_spawn + its tests: dropped (dead, no caller; confirmed zero remaining references).
• except TypeError, ValueError → except ValueError.
• Double lock in should_fan_out_gateway_session: collapsed into one acquisition; pod-mode behavior unchanged.
• Anomaly name single-sourced via EVENT_PUMP_IDLE_BUDGET_ANOMALY; I confirmed the wrapper template still interpolates --anomaly stuck-phase-transition.

🟡 Non-blocking — one residual

_publish_active_roles docstring overstates the reconcile path (event_loop.py:769-772). It claims a reconciled key "self-heals on the next derive." That isn't true: _handle_role returns early at the dedupe check (:819) for an already-live key without populating _key_meta, so a key seeded via reconcile() would stay absent from the published active-role set for the pod's entire lifetime — silently suppressing its tripwires, not self-healing in one tick. This is harmless today because reconcile() is test-only, but it's a public method, and if it were ever wired in production this would be a real coverage gap. Cheap hardening: self._key_meta.setdefault(key, (action, role)) on the dedupe early-return path so adopted/reconciled keys are always labeled. (Also: my prior NB1 question about one-shot pods outliving the gateway's 60-min idle window with fan-out fully suppressed remains a slice-4 spawn-time-refresh concern, out of scope for this delta.)

— Authored by egg

Re-review — clean ✅

The delta since my prior review (8f56c94 → 3ee32b9) is a single change: the _publish_active_roles docstring in orchestrator/event_loop.py. The change is entirely inside the triple-quoted docstring — verified the diff touches no executable line, no tests, and no other file (the large .egg-state/brc-history/* blobs are orchestrator artifacts, out of review scope). So this is wording-only with no behavior change, as the commit message states.

Prior concern addressed

My earlier approve carried exactly one non-blocking note: the docstring's "absent here for exactly one tick" was imprecise. 3ee32b9 rewrites it to describe the real ordering. I traced it against the code to confirm it's accurate, not just reworded:

• Ordering claim holds — poll_once (event_loop.py:742-757) runs the per-role _handle_role pass first, then calls _publish_active_roles. ✓
• Labeling claim holds — the dedupe early-return in _handle_role (event_loop.py:823-829) calls self._key_meta.setdefault(key, (action, role)) before returning, so a reconciled key seeded by reconcile() into _live_keys is labeled on that same tick. ✓
• Fail-safe clause retained — "Were the label ever missing it would only suppress (never false-alert)…" is kept, which correctly covers the narrow residual cases the absolute phrasing doesn't (e.g. an exhausted reconciled key returns at event_loop.py:812 before reaching the labeling at :829, or a reconciled key whose derived action no longer matches the seeded key). ✓

The concern is genuinely fixed, and the docstring now matches the implementation. No new issues in the delta.

Approving.

— Authored by egg

Contract re-verification — PR #3198 (slice-5/6)

Re-reviewed at 3ee32b931b9a7fc9cc925a3f78a1f595f085a379. My prior review was at 8f56c94, which was rebased away — the PR branch now carries a single PR-authored commit over the base (egg/issue-3064/slice-4): 3ee32b93 Correct _publish_active_roles docstring on reconcile tick ordering.

Delta since last review — verified clean

The only change is a docstring correction to OrchestratorEventLoop._publish_active_roles (orchestrator/event_loop.py). I confirmed it is wording-only, no behavior change, and that the new wording is accurate: poll_once runs the _handle_role pass — which labels the dedupe key via _key_meta.setdefault(key, (action, role)) on the in-flight early-return (event_loop.py:824) — before _publish_active_roles() (event_loop.py:756). So a reconcile()-seeded key (_live_keys populated, no _key_meta) is labeled and published on the first published tick, never absent from a real publish. This is directly exercised by TestActiveRolesPublishing::test_reconciled_role_published_via_dedupe_path. No previously-verified criterion is broken by this delta.

Acceptance criteria — re-verified against current code

task-5-1 (coder) — all criteria hold:

• No-pod-never-alerts / active-Job-only scoping / silent mid-event pod trips → health_monitor.py: _orchestrator_skip_tripwire (461-484), gated in check_heartbeats (797-800), check_progress (913-915), and _on_container_stopped (641); silent-pod coverage via the snapshot augmentation that injects active-Job roles with last_hb=0.0 (772-781). ✓
• Pending-event-over-budget raises existing anomaly via EGG_BRC_IDLE_BUDGET_MIN → event_loop.py: _check_convergence_stall (871+), get_idle_budget_minutes, anomaly single-sourced via _idle_budget_anomaly_name → consensus_wrapper.EVENT_PUMP_IDLE_BUDGET_ANOMALY. ✓
• HeartbeatCoordinator mode guard + refresh-at-spawn; pod-mode unchanged → heartbeat.py: set_orchestrator_mode and the if self._orchestrator_mode: return False gate in should_fan_out_gateway_session (production wiring in concurrent_executor._enable_orchestrator_mode_surfaces). Code is correct. ✓ (see coverage note below)
• Existing health/heartbeat tests pass unmodified → all three test diffs are pure additions; no existing test bodies touched. ✓

task-5-2 (tester) — all criteria hold:

• Both ownership modes asserted side-by-side per tripwire → test_health_monitor.py::TestOwnershipModeHeartbeatMatrix covers heartbeat-timeout, container-exit (live vs. ghost), and progress-stall in both modes. ✓
• Idle-budget anomaly-name equality asserted against the in-pod alert's name → test_event_loop.py::TestConvergenceStall::test_alert_fires_once_after_budget asserts call["anomaly"] == EVENT_PUMP_IDLE_BUDGET_ANOMALY == "stuck-phase-transition"; default-equality cross-check in test_health_monitor.py::TestOwnershipModeIdleBudgetAnomaly. ✓
• Existing tests pass unmodified → ✓

Non-blocking concern — heartbeat mode-guard test gap

The HeartbeatCoordinator orchestrator-mode guard (set_orchestrator_mode + the orchestrator-mode short-circuit in should_fan_out_gateway_session) is a real behavior change with no direct test coverage. The added test_heartbeat.py::TestModeGuard::test_absent_sender_in_orchestrator_mode_does_not_trip only calls is_duplicate(...) and its own docstring concedes it does not exercise the guard. Every existing should_fan_out_gateway_session test runs in the default (pod) mode. task-5-2's description prose calls for "mode guard, refresh-at-spawn, absent-sender tolerance, unchanged pod-mode refresh" in test_heartbeat.py, and that is not delivered.

This sits outside task-5-2's formal bulleted acceptance_criteria (which scope to health-monitor tripwires and the idle-budget anomaly name), so it does not block formal contract compliance — but it leaves a behavior-changing branch in heartbeat.py untested. Recommend adding a focused test, e.g.:

def test_orchestrator_mode_suppresses_fan_out(self):
    coord = HeartbeatCoordinator()
    coord.set_orchestrator_mode(True)
    assert coord.should_fan_out_gateway_session("p1", None, "coder", 30.0) is False

def test_pod_mode_fan_out_unchanged(self):
    coord = HeartbeatCoordinator()  # default pod mode
    assert coord.should_fan_out_gateway_session("p1", None, "coder", 30.0) is True
    assert coord.should_fan_out_gateway_session("p1", None, "coder", 30.0) is False  # cooldown

Note on tooling

The orchestrator was unreachable during this review (egg-orch health → UNREACHABLE), so egg-contract verify-criterion could not be run. The contract snapshot also carries no top-level acceptance_criteria (ac-N) entries — the gateable criteria live per-task under slices[slice-5].tasks[].acceptance_criteria, which I verified above.

Verdict: Contract compliance holds; the latest docstring delta is clean and introduces no regression. The single concern above is a non-blocking coverage gap, surfaced for the human reviewer / coder.

— Authored by egg

Re-review — delta since 3ee32b9 ✅

The delta since my last review is a single, purely-additive test commit (0934b79, +44 lines in orchestrator/tests/test_heartbeat.py, no production changes). It addresses the residual non-blocking note that should_fan_out_gateway_session's orchestrator-mode guard had no direct test coverage.

Verified the new tests exercise the production path:

• All three tests (test_orchestrator_mode_suppresses_fan_out, test_pod_mode_fan_out_unchanged, test_orchestrator_mode_toggle_restores_pod_behavior) instantiate a real HeartbeatCoordinator and call the real set_orchestrator_mode() / should_fan_out_gateway_session() — no fixtures bypass the helper, no self-seeding.
• Assertions match the production logic in heartbeat.py:171–189: orchestrator mode short-circuits to False before the cooldown bookkeeping (so it never records a timestamp — correctly asserted by the double-call in test 1), pod mode records on first call and suppresses the second within the 30s window, and the False toggle restores pod-mode fan-out. The single-lock refactor (one with self._lock: covering both the gate and the cooldown) is consistent with what these tests drive.
• Test names match behavior — no name-vs-behaviour contradictions.
• Ran pytest orchestrator/tests/test_heartbeat.py::TestModeGuard — 4 passed.

All blocking issues from my initial review (set_active_roles wiring / active-Job scoping, the convergence-stall behavioral test, the container-exit gate consistency, and the vacuous anomaly-name assertion) were resolved in the 6b5cf0dc→3ee32b9 commits and approved in my prior re-reviews. Nothing in this delta regresses them.

No blocking or non-blocking issues in the new code. Approving.

— Authored by egg

Contract re-verification — PR #3198 (issue-3064 slice-5/6)

Re-reviewed at 0934b794914f0279bc07342eef6ba931e33f866d. My prior review was at 3ee32b93. Since then the PR branch carries a single PR-authored commit over the base (egg/issue-3064/slice-4):

• 0934b794 Add direct mode-guard coverage for should_fan_out_gateway_session

Scope reviewed: git log 3ee32b93..0934b794 --not origin/egg/issue-3064/slice-4 -p → 1 file, +44/-0, test-only (orchestrator/tests/test_heartbeat.py).

Verdict: clean — no contract violations, no regressions

The delta is purely additive test coverage and addresses the prior review NB that the HeartbeatCoordinator orchestrator-mode guard in should_fan_out_gateway_session had no direct test coverage. Three new TestModeGuard methods were added:

1. test_orchestrator_mode_suppresses_fan_out — asserts set_orchestrator_mode(True) makes should_fan_out_gateway_session return False, and that a second call is still False (no cooldown timestamp is ever recorded in orchestrator mode).
2. test_pod_mode_fan_out_unchanged — asserts default pod mode keeps the cooldown-gated behavior (first call fans out + records, second within cooldown is suppressed).
3. test_orchestrator_mode_toggle_restores_pod_behavior — asserts set_orchestrator_mode(False) un-gates the fan-out path again.

Verification against production (orchestrator/heartbeat.py:171-201): the assertions match the implementation exactly. The orchestrator-mode early-return (if self._orchestrator_mode: return False, line 182-183) precedes the cooldown bookkeeping (self._last_fan_out[key] = now, line 187), so "suppressed and never records a timestamp" is correct; the pod-mode path is the unchanged now - last < min_interval_seconds cooldown gate; and set_orchestrator_mode toggles the flag under the lock. The tests are accurate, not tautological.

Tests pass (run locally, targeted):

• pytest orchestrator/tests/test_heartbeat.py::TestModeGuard → 4 passed
• pytest orchestrator/tests/test_heartbeat.py (full file) → 31 passed

This directly strengthens task-5-2 coverage ("HeartbeatCoordinator mode guard") and the task-5-1 AC3 mode-guard clause. No production code changed in this delta, so no previously-verified acceptance criterion can regress.

Note on criterion marking

The orchestrator was unreachable for the duration of this review (egg-orch health → Status: UNREACHABLE), so egg-contract verify-criterion could not be invoked to persist criterion state. This does not change the verification conclusion above — the substantive slice-5 criteria were already adjudicated in prior incremental reviews, and this test-only delta introduces no new completed task and no regression. Re-run criterion marking once the orchestrator recovers if any AC remains unpersisted.

— Authored by egg

Contract Re-Verification — PR #3198 (slice-5)

Verdict: ACK / approve-equivalent (posted as a comment — self-authored bot PR).

Scope of this re-review

Last reviewed at 0934b794. That commit had been rewritten out of the branch history (the local checkout was shallow); after git fetch --deepen I recovered it and computed the precise delta.

The only change since my last review is the merge commit 80f4c0f8 (Merge origin/egg/issue-3064/slice-4 into slice-5), parents 0934b794 (prior slice-5) and 5091c1e4 (slice-4 HEAD). It resolved conflicts in two files. No new slice-5 logic was authored.

Conflict-resolution audit (the actual delta)

orchestrator/concurrent_executor.py — purely additive, verified via the combined merge diff:

• slice-5's _publish_active_roles + _enable_orchestrator_mode_surfaces and slice-4's owns_event_loop + stop_event_loop all coexist as four distinct methods, each defined exactly once.
• Wiring intact: active_roles_notifier=self._publish_active_roles (concurrent_executor.py:510) and self._enable_orchestrator_mode_surfaces() (:527) preserved; slice-4's owns_event_loop()/stop_event_loop() call sites at the completion-poll site (routes/pipelines.py:18436,18912,19229) preserved.

orchestrator/tests/test_event_loop.py — purely additive:

• slice-5's TestConvergenceStall (:1378) + TestActiveRolesPublishing (:1561) and slice-4's TestAdoptionTimingSuppression (:1637) each present exactly once. The merge-authored content is only the restored section-header delimiter (whitespace/comment).

Both files parse cleanly (AST check).

Regression check on previously-verified criteria

• orchestrator/health_monitor.py: 0 diff lines vs 0934b794 — byte-identical. Mode-aware tripwire behavior (TASK-5-1) unchanged.
• orchestrator/heartbeat.py: 0 diff lines vs 0934b794 — byte-identical. Coordinator mode-guard (TASK-5-1) unchanged.
• orchestrator/event_loop.py: changed only by slice-4's cross-process adoption logic (spawn_result is None ⇒ spawned=False, no timing) — additive, non-conflicting, and matched by slice-4's own TestAdoptionTimingSuppression. slice-5's convergence-stall + active-roles-publishing hooks (_check_convergence_stall, convergence_stall_notifier, active_roles_notifier, _stall_first_seen/_stall_alerted, the poll_once call at :739) are all intact.

No previously-verified acceptance criterion regressed; no contract violation introduced by the merge.

Note

The orchestrator is unreachable from this session, so egg-contract verify-criterion cannot write criterion status. slice-5's criteria were verified at the prior cycle and nothing in this delta regresses them; that verification stands.

— Authored by egg

Re-review — PR #3198 (issue-3064 slice-5/6), conflict-resolution merge

Prior review at 0934b794. Since then the only PR-authored commit over the base (egg/issue-3064/slice-4) is the merge commit 80f4c0f8:

git log 0934b794..HEAD --not origin/egg/issue-3064/slice-4 --oneline
80f4c0f8c Merge origin/egg/issue-3064/slice-4 into slice-5 …

Everything else in 0934..HEAD is slice-4 base content brought in by the merge (excluded). The reviewable delta is the conflict resolution in two files.

Verdict: clean — purely-additive merge, no blocking or non-blocking issues

orchestrator/concurrent_executor.py — verified both sides preserved as four distinct methods, each defined exactly once (no duplicate definitions):

• slice-5: _publish_active_roles (531), _enable_orchestrator_mode_surfaces (553)
• slice-4: owns_event_loop (582), stop_event_loop (598)

The slice-5 notifier wiring in spawn_all survived (convergence_stall_notifier=self._emit_supervision_alert, active_roles_notifier=self._publish_active_roles, lines 509-510) and _enable_orchestrator_mode_surfaces() is still called (527). The convergence_stall_notifier kwargs (anomaly/priority/summary/detail) match _emit_supervision_alert's keyword-only signature exactly (941-943) — type-compatible. slice-4's stop_event_loop/owns_event_loop call sites in routes/pipelines.py (18436, 18912, 19229) are intact.

orchestrator/tests/test_event_loop.py — both test bodies preserved: slice-5's TestConvergenceStall + TestActiveRolesPublishing (+ _NotifierSpy/_StallTracker helpers) and slice-4's _AdoptingSpawner + TestAdoptionTimingSuppression. The collapsed # --- header delimiter for the Slice-3 section reads correctly (572-580); no orphaned comment block.

Semantic-merge check (the real risk in a merge, not textual conflicts)

• event_loop.py — slice-4's cross-process adoption change (spawn_result is None → return spawned=False) composes correctly with slice-5's _publish_active_roles: self._key_meta[key] = (action, role) is set before the adoption return (850-862), so adopted keys are labeled and published, and _check_convergence_stall's in-flight suppression reads _live_keys which the adoption path populates. No interaction defect. slice-5's _check_convergence_stall, get_idle_budget_minutes, _idle_budget_anomaly_name, and the _handle_role dedupe-path setdefault label are all intact.
• health_monitor.py / heartbeat.py — git diff 0934..HEAD is empty for both; slice-4 did not touch them, so slice-5's monitoring content is byte-identical to what I approved at 0934.

Build verification

• python -m py_compile clean on event_loop.py, concurrent_executor.py, health_monitor.py, heartbeat.py, and the three changed test files.
• No duplicate symbol definitions for any merged method.

Per workflow constraints I did not run make test (CI gates this review on a green check suite). The resolution matches the author's conflict-resolution summary, and I independently confirmed it is correct.

— Authored by egg